-
Notifications
You must be signed in to change notification settings - Fork 0
docs: #48 Final M2 documentation cleanup #53
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked this against the code rather than reading it as prose. Most of it is accurate. Two places contradict what the code does, and one of them is the first line of the README.
What I verified as correct
AVERAGE_RATING_DESCreally does fail.server/src/search/search.tsthrowsGraphQLError('Sorting by rating is not available until reviews are stored.'). This is a better statement than the "four sort modes" I left in the README, so it corrects me and I want to say so plainly rather than let it pass silently.X-User-Idis defined but never read.server/srconly has theuserIdcolumns indb/schema.ts. Matches the text.- The frontend really requests
first: 20withsort: 'RELEVANCE'inclient/src/hooks/useAlbumSearch.ts. db:seeddestroying reviews and crate items throughTRUNCATE ... CASCADEis whatseed.tsdoes.- Sample catalog numbers hold: 16 albums,
bluematches 4, Rock 7 / Pop 3 / Electronic 2, genre id 4 is Rock. - The CI description matches
.github/workflows/ci.yml, including the migration step and the checkout to~/T19-Project-2at$GITHUB_SHA.
Two fixes before this merges
1. README.md:5 contradicts the Implemented section.
Crate is an album catalog where users can search for music. The planned application also includes filtering, sorting, album details, ratings, reviews and a personal wantlist ("crate").
The same file says further down that the GraphQL backend supports "genre and decade filters, sorting and cursor-based pagination". So the opening sentence tells the reader those do not exist anywhere, which is the opposite of what this PR sets out to clarify. The split is API now, UI later, so something like "the API already supports filtering, sorting and cursor-based pagination; the controls for those in the interface are planned" keeps the two sections consistent.
2. docs/VM-setup.md:116 drops the decadeFacets expectation but keeps the field.
The query on line 121 still selects decadeFacets { decade count }, but the rewritten text only gives totalCount: 7 and the genre facet values. The previous version said decadeFacets counts only Rock albums, which gave whoever runs the suite something to check that half of the response against. Either restore that sentence or drop decadeFacets from the selection.
Why I am not approving yet
Both are one-sentence fixes, and the rest of this is solid. But a fix needs a push, and a push dismisses an approval, so approving now would only cost you a second round trip. Fix these two and I will approve.
I have not marked either as blocking in the technical sense. The first one matters more than its size suggests, because it is the opening line of the file people actually read and it undercuts the point of the PR.
|
Thanks for the thorough review! I've fixed both points: clarified the API/frontend distinction in the README and restored the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both fixes are correct, and they are exactly what I asked for. Approving.
README intro now draws the line where the code actually draws it: the API has filtering, sorting and cursor-based pagination, the interface controls for those are planned. That matches the Implemented and Planned sections further down, so the file no longer contradicts itself on its first line.
VM-setup test 4 is back to having a decadeFacets expectation to check against.
One observation, and I want to be clear it is not blocking and is not something you introduced. I checked the seed data to see what a tester would actually get for that query:
| Album | Year | Decade |
|---|---|---|
| Abbey Road | 1969 | 1960 |
| Revolver | 1966 | 1960 |
| The Dark Side of the Moon | 1973 | 1970 |
| Rumours | 1977 | 1970 |
| OK Computer | 1997 | 1990 |
| Kid A | 2000 | 2000 |
| Untitled Demo Tape | null |
none |
So totalCount: 7 but the decades sum to 6, because Untitled Demo Tape is Rock with a null year and falls into no bucket. Whoever runs the suite and adds up decadeFacets gets 6 next to a totalCount of 7 and may wonder whether the check failed.
The sentence you wrote is accurate as it stands. If you want to make it harder to misread, the concrete numbers would be 1960: 2, 1970: 2, 1990: 1, 2000: 1 and a note that one Rock album has no year. Entirely your call. Merge as-is if you would rather not touch it again.
For the record on the earlier round: the AVERAGE_RATING_DESC wording is still a correction of mine and a better statement than what I had left behind.
Summary
Final documentation cleanup for M2 across seven files.
Testing
pnpm formatpnpm format:check— passedgit diff --check— passedCloses #48